-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Extend time range bucketing attributes to retrievers #136072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend time range bucketing attributes to retrievers #136072
Conversation
There's situations where the time range filter is provided as part of the retriever tree. In that case, we capture the time range filter from while parsing it, but we don't do the corresponding introspection of the retriever tree to extract which fields were the time range filters made against. This commit introduces a very basic introspection of retrievers and tests around it.
Hi @javanna, I've created a changelog YAML for you. |
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
} | ||
} | ||
|
||
private static void introspectRetriever(RetrieverBuilder retrieverBuilder, QueryMetadataBuilder queryMetadataBuilder, int level) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not particularly happy about having to make this change. I think that retrievers with a time range filter are a bit of an edge case that is not so important for our current metrics effort around search performance.
That said, this change is required to avoid weird discrepancies due to the very general way that we track the time range filter from, without extensive knowledge of where we get it from. We could disable tracking it in certain situations as an alternative, but retrievers get rewritten to ordinary queries hence at the shard level we don't have the info to be able to make the distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Luca
private static void assertTimeRangeAttributes(List<Measurement> measurements, String target, boolean isSystem) { | ||
public void testTimeRangeFilterRetrieverOneResult() { | ||
SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder(); | ||
searchSourceBuilder.retriever(new StandardRetrieverBuilder(new RangeQueryBuilder("@timestamp").from("2024-12-01"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice test, thank you !
There's situations where the time range filter is provided as part of the retriever tree. In that case, we capture the time range filter from while parsing it, but we don't do the corresponding introspection of the retriever tree to extract which fields were the time range filters made against.
This commit introduces a very basic introspection of retrievers and tests around it, expanding on #135549.